Add more reactivity test for alpine, preact, ember, lit, svelte - #6462
Conversation
📝 WalkthroughWalkthroughThe PR adds grouped-cell rendering support across adapters, Ember owner lifecycle cleanup, Lit renderable typing, Svelte option synchronization, Preact exotic-component handling, and broad framework-specific reactivity, rendering, lifecycle, and SSR tests. ChangesAdapter rendering and reactivity
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7952940 to
619d1af
Compare
|
View your CI Pipeline Execution ↗ for commit 619d1af
☁️ Nx Cloud last updated this comment at |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We changed BoundFlexRenderComponent from Component<Record<string, never>> to Component<Record<string, any>> to fix the TypeScript type errors in the Svelte example apps. Record<string, never> made every prop value an impossible never type, causing TypeScript to reject the Header_Core<...> values passed via <header.FlexRender header={header} />. Our fix correctly represents a pre-bound component that can accept any props.
Tip
✅ We verified this fix by re-running tanstack-svelte-table-example-composable-tables:test:types.
diff --git a/packages/svelte-table/src/createTableHook.svelte.ts b/packages/svelte-table/src/createTableHook.svelte.ts
index afe06809..c25799d2 100644
--- a/packages/svelte-table/src/createTableHook.svelte.ts
+++ b/packages/svelte-table/src/createTableHook.svelte.ts
@@ -38,7 +38,7 @@ import type {
} from '@tanstack/table-core'
export type ComponentType<T extends Record<string, any>> = Component<T>
-export type BoundFlexRenderComponent = Component<Record<string, never>>
+export type BoundFlexRenderComponent = Component<Record<string, any>>
function createLiveView<TCurrent extends object, TExtensions extends object>(
getCurrent: () => TCurrent,
Or Apply changes locally with:
npx nx-cloud apply-locally eiWd-HgmI
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/ember-table/tests/integration/external-state.test.gts (1)
763-766: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the table outside the tracked getter.
Assigning
capturedTableas a side effect of a tracked getter couples the capture to render timing and makes the latercapturedTable!assertions implicitly dependent on the template having rendered. A field-level capture is clearer and equally valid.♻️ Suggested refactor
class TableComponent extends Component { table = useTable(this, () => ({ data: makeData(...UNSORTED.map((firstName) => ({ firstName }))), columns, features, atoms: { pagination: paginationAtom }, })) + constructor(...args: ConstructorParameters<typeof Component>) { + super(...args) + capturedTable = this.table + } + get pageSize() { - capturedTable = this.table return this.table.store.state.pagination.pageSize }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ember-table/tests/integration/external-state.test.gts` around lines 763 - 766, Move the capturedTable assignment out of the tracked pageSize getter and initialize it at field level when the table instance is created. Keep the getter limited to returning this.table.store.state.pagination.pageSize, while preserving the existing capturedTable assertions.packages/lit-table/src/createTableHook.ts (1)
778-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute header/footer through
FlexRendertoo, for symmetry withAppCell.
cellFlexRendernow delegates toFlexRender({ cell })(Line 747), but header/footer still callflexRender(...)directly. Behavior is identical today (FlexRender's header/footer branches do exactly this perpackages/lit-table/src/flexRender.ts:131-144), so this is purely about keeping one rendering entry point asFlexRendergrows special cases.♻️ Suggested refactor
- const headerFlexRender = () => - flexRender(header.column.columnDef.header, header.getContext()) + const headerFlexRender = () => FlexRender({ header })- const footerFlexRender = () => - flexRender(header.column.columnDef.footer, header.getContext()) + const footerFlexRender = () => FlexRender({ footer: header })Also applies to: 808-809
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lit-table/src/createTableHook.ts` around lines 778 - 779, Update the header and footer render helpers in createTableHook to delegate through FlexRender using the appropriate header/footer props, matching cellFlexRender’s FlexRender({ cell }) pattern. Replace the direct flexRender calls while preserving the existing header and footer contexts and output behavior.packages/lit-table/src/subscribe-directive.ts (1)
155-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate render logic with
createFakeHost().requestUpdate.The value-render snippet here is identical to the one in
requestUpdate(Lines 171-173). Extract a small private helper to keep both call sites in sync going forward.♻️ Suggested extraction
+ private renderCurrentValue(): void { + if (this.resolvedTemplate && this.controller) { + this.setValue(this.resolvedTemplate(this.controller.value)) + } + } + /** Restores the controller subscription when the directive is re-attached to the DOM. */ reconnected() { this.controller?.hostUpdate() - if (this.resolvedTemplate && this.controller) { - this.setValue(this.resolvedTemplate(this.controller.value)) - } + this.renderCurrentValue() }requestUpdate: () => { - if (this.resolvedTemplate && this.controller) { - this.setValue(this.resolvedTemplate(this.controller.value)) - } + this.renderCurrentValue() },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lit-table/src/subscribe-directive.ts` around lines 155 - 159, Extract the duplicated resolved-template rendering logic from reconnected() and requestUpdate into a small private helper, preserving the existing guards and calls to setValue with controller.value. Replace both call sites with the helper so future changes remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ember-table/tests/integration/external-state.test.gts`:
- Around line 763-766: Move the capturedTable assignment out of the tracked
pageSize getter and initialize it at field level when the table instance is
created. Keep the getter limited to returning
this.table.store.state.pagination.pageSize, while preserving the existing
capturedTable assertions.
In `@packages/lit-table/src/createTableHook.ts`:
- Around line 778-779: Update the header and footer render helpers in
createTableHook to delegate through FlexRender using the appropriate
header/footer props, matching cellFlexRender’s FlexRender({ cell }) pattern.
Replace the direct flexRender calls while preserving the existing header and
footer contexts and output behavior.
In `@packages/lit-table/src/subscribe-directive.ts`:
- Around line 155-159: Extract the duplicated resolved-template rendering logic
from reconnected() and requestUpdate into a small private helper, preserving the
existing guards and calls to setValue with controller.value. Replace both call
sites with the helper so future changes remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df42cc5a-2428-4832-a9b1-e3df0293c972
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
knip.jsonpackages/alpine-table/package.jsonpackages/alpine-table/src/flexRender.tspackages/alpine-table/tests/unit/adapterCoverage.test.tspackages/alpine-table/tsconfig.jsonpackages/ember-table/src/FlexRender.gtspackages/ember-table/src/create-table-hook.tspackages/ember-table/src/use-table.tspackages/ember-table/tests/integration/create-table-hook.test.gtspackages/ember-table/tests/integration/external-state.test.gtspackages/ember-table/tests/integration/flex-render.test.gtspackages/ember-table/tests/integration/reactivity.test.gtspackages/lit-table/package.jsonpackages/lit-table/src/TableController.tspackages/lit-table/src/createTableHook.tspackages/lit-table/src/flexRender.tspackages/lit-table/src/subscribe-directive.tspackages/lit-table/tests/unit/adapterLifecycle.test.tspackages/lit-table/tests/unit/defaultReactivity.test.tspackages/lit-table/tests/unit/flexRender.test.tspackages/lit-table/tests/unit/rendering.test.tspackages/lit-table/tests/unit/selectorGate.test.tspackages/lit-table/tsconfig.jsonpackages/preact-table/package.jsonpackages/preact-table/src/FlexRender.tsxpackages/preact-table/tests/unit/adapterReactivity.test.tsxpackages/preact-table/tests/unit/createTableHook.test.tsxpackages/preact-table/tests/unit/rendering.test.tsxpackages/preact-table/tests/unit/ssr.test.tsxpackages/preact-table/tests/unit/useTable.test.tsxpackages/react-table/tests/adapterReactivity.test.tsxpackages/svelte-table/package.jsonpackages/svelte-table/src/FlexRender.sveltepackages/svelte-table/src/createTable.svelte.tspackages/svelte-table/tests/adapter-lifecycle.test.tspackages/svelte-table/tests/fixtures/CallbackHarness.sveltepackages/svelte-table/tests/fixtures/ContextFailure.sveltepackages/svelte-table/tests/fixtures/FlexRenderHarness.sveltepackages/svelte-table/tests/fixtures/HookCellBadge.sveltepackages/svelte-table/tests/fixtures/HookHarness.sveltepackages/svelte-table/tests/fixtures/HookHeaderBadge.sveltepackages/svelte-table/tests/fixtures/HookTableBadge.sveltepackages/svelte-table/tests/fixtures/PaginationHarness.sveltepackages/svelte-table/tests/fixtures/ReactivityHarness.sveltepackages/svelte-table/tests/fixtures/RenderBadge.sveltepackages/svelte-table/tests/fixtures/SelectorHarness.sveltepackages/svelte-table/tests/fixtures/SsrHarness.sveltepackages/svelte-table/tests/fixtures/hook-fixture.tspackages/svelte-table/tests/rendering.test.tspackages/svelte-table/tests/ssr.test.tspackages/svelte-table/tests/test-setup.tspackages/svelte-table/tsconfig.jsonpackages/svelte-table/vite.config.ts
Added more test for alpine, preact, ember, lit, svelte. Also this fixes the missing
cleanupin ember adapter integrationEmber fixes
Improved Ember adapter lifecycle handling by allowing
useTableandcreateAppTableto receive an Ember owner. Table reactivity is now automatically cleaned up when that owner is destroyed, preventing external atoms and destroyed tables from continuing to update each other.The existing owner-less API remains supported for standalone usage. Tests cover cleanup in both update directions, external atom precedence, state synchronization, and rendering behavior.
Summary by CodeRabbit
New Features
Bug Fixes